Skip to content

Review system requirements #4070

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 16 commits into from
Aug 29, 2025
Merged

Review system requirements #4070

merged 16 commits into from
Aug 29, 2025

Conversation

@aneta-petrova
Copy link
Member

@aneta-petrova aneta-petrova commented Jul 26, 2025

What changes are you introducing?

Streamlining the system requirements a bit, to phrase them as actionable steps and remove some duplicates I noticed.

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

The goal is to make the requirements easier to follow and to turn them from lengthy-ish descriptions into a sort of a quick "pre-flight" checklist.

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

This started with my noticing that we advise installing on a fresh system in 3 different places. What was supposed to be a very quick PR to consolidate that, turned into a slightly larger PR along the lines "while I'm here, why don't I try fixing up the whole section..."

Contributor checklists

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.15/Katello 4.17
  • Foreman 3.14/Katello 4.16 (Satellite 6.17)
  • Foreman 3.13/Katello 4.15 (EL9 only)
  • Foreman 3.12/Katello 4.14 (Satellite 6.16; orcharhino 7.2 on EL9 only; orcharhino 7.3)
  • Foreman 3.11/Katello 4.13 (orcharhino 6.11 on EL8 only; orcharhino 7.0 on EL8+EL9; orcharhino 7.1 with Leapp)
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9/6.10)
  • We do not accept PRs for Foreman older than 3.9.

Review checklists

Tech review (performed by an Engineer who did not author the PR; can be skipped if tech review is unnecessary):

  • The PR documents a recommended, user-friendly path.
  • The PR removes steps that have been made unnecessary or obsolete.
  • Any steps introduced or updated in the PR have been tested to confirm that they lead to the documented end result.

Style review (by a Technical Writer who did not author the PR):

  • The PR conforms with the team's style guidelines.
  • The PR introduces documentation that describes a user story rather than a product feature.

@github-actions github-actions bot added Needs tech review Requires a review from the technical perspective Needs style review Requires a review from docs style/grammar perspective Needs testing Requires functional testing labels Jul 26, 2025
@aneta-petrova aneta-petrova removed the Needs testing Requires functional testing label Jul 26, 2025
@github-actions
Copy link

github-actions bot commented Jul 26, 2025 •
edited
Loading

The PR preview for 1abac58 is available at theforeman-foreman-documentation-preview-pr-4070.surge.sh

The following output files are affected by this PR:

show diff

show diff as HTML

@aneta-petrova aneta-petrova force-pushed the review-system-requirements branch 2 times, most recently from df5101b to 1fa2eba Compare July 26, 2025 20:20
aneta-petrova
aneta-petrova commented Jul 26, 2025
View reviewed changes
@aneta-petrova
Copy link
Member Author

aneta-petrova commented Jul 28, 2025

Hi @evgeni, can you take a look at these updated installation system prerequisites? Please let me know if:

  1. they are okay for the standard installer
  2. the list applies to the new installer as well

The next step for me would be to reuse these prerequisites in the new installer guide, which is why I'm specifically mentioning 2. as well.

maximiliankolb
maximiliankolb reviewed Jul 28, 2025
View reviewed changes
@aneta-petrova aneta-petrova force-pushed the review-system-requirements branch from 4b57ffe to c680482 Compare July 30, 2025 14:33
@maximiliankolb maximiliankolb added the Waiting on contributor Requires an action from the author label Aug 5, 2025
@aneta-petrova aneta-petrova force-pushed the review-system-requirements branch from c680482 to 7f6bf04 Compare August 27, 2025 08:45
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author Needs re-review labels Aug 27, 2025
evgeni
evgeni reviewed Aug 27, 2025
View reviewed changes
Lennonka
Lennonka requested changes Aug 27, 2025
View reviewed changes
Copy link
Contributor

@Lennonka Lennonka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeeeesss!
Great job, just a few nitpicks below.

@pr-processor pr-processor bot added the Waiting on contributor Requires an action from the author label Aug 27, 2025
ekohl
ekohl reviewed Aug 27, 2025
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I certainly like this new style a lot more.

I was looking at the headers as well. We have "1. Planning Foreman server installation" but then most have "requirements" in the title. "Supported operating systems" is an exception for all, but Katello also has "Best practices for optimizing storage". Have you had a look at that, or is it out of scope for here?

Overall I'm wondering if "Supported operating systems" really is a System requirement. If you don't merge it in, I'd consider listing it before the system requirements.

@aneta-petrova
Copy link
Member Author

aneta-petrova commented Aug 28, 2025

I was looking at the headers as well. We have "1. Planning Foreman server installation" but then most have "requirements" in the title. "Supported operating systems" is an exception for all, but Katello also has "Best practices for optimizing storage". Have you had a look at that, or is it out of scope for here?

Right now, I'm going section by section (or at least topic by topic, if one topic is spread across multiple sections for example). Storage requirements + Best practices for storage were merged in #4096

After I'm done going section by section, I want to look at all the sections together in #4087 That will be a better time review if the sections and their headings play nicely together.

Overall I'm wondering if "Supported operating systems" really is a System requirement. If you don't merge it in, I'd consider listing it before the system requirements.

Merging it with system requirements is a good idea but because it likely wouldn't be a simple 1-2 sentence bullet point (the section actually contains quite a lot of text for Satellite), I'd like to look into it in another PR.

@aneta-petrova
Copy link
Member Author

aneta-petrova commented Aug 28, 2025

Thanks so much for your reviews @ekohl @evgeni @maximiliankolb @Lennonka! I believe I've applied all your feedback and now I'd like to proceed to collecting the acks :) Do you think you could help with that? I'm happy to address any other comments you might have to make sure we get this right.

maximiliankolb
maximiliankolb requested changes Aug 28, 2025
View reviewed changes
@pr-processor pr-processor bot added the Waiting on contributor Requires an action from the author label Aug 28, 2025
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Aug 28, 2025
maximiliankolb
maximiliankolb approved these changes Aug 28, 2025
View reviewed changes
Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style-wise LGTM.

@maximiliankolb maximiliankolb added style review done No issues from docs style/grammar perspective and removed Needs style review Requires a review from docs style/grammar perspective labels Aug 28, 2025
@aneta-petrova aneta-petrova mentioned this pull request Aug 28, 2025
Merged
15 tasks
ekohl
ekohl approved these changes Aug 28, 2025
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 but consider #4070 (comment) as an enhancement.

Lennonka
Lennonka approved these changes Aug 28, 2025
View reviewed changes
Copy link
Contributor

@Lennonka Lennonka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, but I live Ewoud's suggestion.

evgeni
evgeni approved these changes Aug 29, 2025
View reviewed changes
Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@aneta-petrova aneta-petrova added tech review done No issues from the technical perspective and removed Needs tech review Requires a review from the technical perspective labels Aug 29, 2025
@aneta-petrova aneta-petrova merged commit 714d853 into theforeman:master Aug 29, 2025
9 of 10 checks passed
aneta-petrova added a commit that referenced this pull request Aug 29, 2025
@aneta-petrova @maximiliankolb @ekohl
Review system requirements (#4070)
d8815b7 
* Rephrase system requirements into actionable steps
* Drop duplicate information
* Include host name requirement for all builds
* Update KBase link
* Set lower proxy min memory when without content
* Drop hostname requirement for clients
* Drop processing power requirement

---------

Co-authored-by: Maximilian Kolb <[email protected]>
Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
(cherry picked from commit 714d853)
@aneta-petrova
Copy link
Member Author

aneta-petrova commented Aug 29, 2025

Merged to "master" and cherry-picked:

c5157ba..d8815b7 3.16 -> 3.16

@aneta-petrova aneta-petrova deleted the review-system-requirements branch August 29, 2025 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ehelms ehelms ehelms left review comments

@evgeni evgeni evgeni approved these changes

@ekohl ekohl ekohl approved these changes

@maximiliankolb maximiliankolb maximiliankolb approved these changes

@Lennonka Lennonka Lennonka approved these changes

Assignees

No one assigned

Labels

style review done No issues from docs style/grammar perspective tech review done No issues from the technical perspective

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@aneta-petrova @evgeni @ekohl @ehelms @maximiliankolb @Lennonka